Fix inversion of conditional #4685

weltkante · 2021-03-14T18:26:11Z

Contributes to #4370

Proposed changes

This regression to Desktop Framework was found while investigating #4370. It is unclear what the effects of this regression are, but comparing with Desktop Framework it seems its a mistake from refactoring

When running a debug build of WinForms during investigation of #4370 I had turned on CompModSwitches.ActiveX and the broken code resulted in warnings of the ActiveX host passing wrong flags (which of course is wrong, using MFC or Office as host you can assume they pass the right flags).

Customer Impact

unknown, probably changing behavior of hosted controls due to wrong flags being passed to host

Regression?

yes

Risk

low, restoring code to how it was before. There was another recent regression fix for Hosting UserControl via ActiveX does not work correctly #4370 so this should be fine to accompany the other fix

Before

running the sample from Hosting UserControl via ActiveX does not work correctly #4370 (or generally, hosting a WinForms control in a MFC app or other ActiveX host) outputs warnings when using a WinForms debug build with ActiveX tracing turned on

After

no warnings in debug build

Test methodology

source inspection
running the sample from Hosting UserControl via ActiveX does not work correctly #4370 with a debug build of WinForms, making sure the previously observed warning is gone

Test environment(s)

MFC application (stock app from wizard) initializing the WinForms control from the sample code
Outlook 16.0.13801.20240 (32 bit) hosting the WinForms control from the sample code

Microsoft Reviewers: Open in CodeFlow

weltkante · 2021-03-14T18:29:03Z

@RussKie while investigating #4370 I figured out how to host WinForms controls as ActiveX in MFC (which was mostly a problem of MFC being underdocumented and the web starting to forget a lot of docs/examples from the pre- .NET Desktop Framework era; the Desktop Framework docs don't apply since it had special integration via C++/CLI which does not exist for .NET Core so you have to code like you would for non-.NET ActiveX controls, which unfortunately aren't documented well anymore).

If you think we should add C++/MFC unit tests covering hosting WinForms controls then I could write some code for that. Really depends on how you want to treat ActiveX compatibility going forward.

This particular regression is hard to unit test though, as the method is never called by user code, so any ActiveX unit testing probably should be done separately.

codecov · 2021-03-14T18:58:00Z

Codecov Report

Merging #4685 (163e616) into main (99eac12) will decrease coverage by 0.00025%.
The diff coverage is n/a.

❗ Current head 163e616 differs from pull request most recent head 0c160b7. Consider uploading reports for the commit 0c160b7 to get more accurate results

@@                 Coverage Diff                 @@
##                main       #4685         +/-   ##
===================================================
- Coverage   97.96413%   97.96388%   -0.00025%     
===================================================
  Files            548         543          -5     
  Lines         266323      264375       -1948     
  Branches        5117        4970        -147     
===================================================
- Hits          260901      258992       -1909     
+ Misses          4529        4499         -30     
+ Partials         893         884          -9

Flag	Coverage Δ
Debug	`97.96388% <ø> (-0.00025%)`	⬇️
test	`97.96388% <ø> (-0.00025%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

hughbe · 2021-03-15T10:04:17Z

If you think we should add C++/MFC unit tests covering hosting WinForms controls then I could write some code for that. Really depends on how you want to treat ActiveX compatibility going forward.

Could you share your example? Would love to try testing this out too!

weltkante · 2021-03-15T12:52:08Z

@hughbe sure, I've created a cleaned up sandbox project here and attached the instructions to create a new project below. Note that you need a custom WinForms build or a recent nightly to get the other (already merged) regression fix from #4370, otherwise the control will not show up.

The application breaks into an MFC assert when terminating, I didn't look into that so I don't know who is at fault here, but I guess .NET still has a reference back to a COM interface that hasn't been garbage collected yet.

showing an ActiveX control in an MFC application

For the managed project:

use specific bitness to match the native host, e.g. <PlatformTarget>x86</PlatformTarget> if the host is building as x86
add <EnableComHosting>true</EnableComHosting> to get the .comhost.dll generated
create a control you want to expose, make it ComVisible and give it a Guid attribute
declare an interface and link it via ComDefaultInterface to get IDispatch support (see this comment for details)
(this may not be necessary for very basic operation but it is probably a good idea to do anyways)

For the native application:

Create a new "MFC App" project, mostly with default settings
- either "Single Document" or "Multiple Documents"
- switching to project style "MFC Standard" instead of keeping the default since it simplifies the app
- under advanved features you can disable some more if you want to
  - turning off printing and print preview
  - turning off restart manager support

in the generated view class add

CWnd m_control;
BOOL Create(LPCTSTR lpszClassName, LPCTSTR lpszWindowName, DWORD dwStyle,
  const RECT& rect, CWnd* pParentWnd, UINT nID, CCreateContext* pContext = NULL) override;

in the generated view code add with your controls guid

BOOL CMFCApplication1View::Create(LPCTSTR lpszClassName, LPCTSTR lpszWindowName, DWORD dwStyle, const RECT& rect, CWnd* pParentWnd, UINT nID, CCreateContext* pContext)
{
  CLSID clsid;
  return CView::Create(lpszClassName, lpszWindowName, dwStyle, rect, pParentWnd, nID, pContext)
    && SUCCEEDED(IIDFromString(L"{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}", &clsid))
    && m_control.CreateControl(clsid, NULL, WS_VISIBLE, CRect(0, 0, 200, 200), this, 0);
}

Using IIDFromString since I only want to parse a GUID and CLSIDFromString does various other auto translations

add an app.manifest with the controls guid to allow running the app without registering the .comhost.dll globally

<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
  <assemblyIdentity version="1.0.0.0" name="MyApplication.app"/>
  <file name="ClassLibrary1.comhost.dll">
    <comClass clsid="{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}" threadingModel="Both" />
  </file>
</assembly>

Note that nothing else needs to be in this manifest, VC++ will merge it with the autogenerated manifest.

RussKie · 2021-03-29T01:27:25Z

Regression introduced in #2093

RussKie · 2021-03-29T01:29:36Z

If you think we should add C++/MFC unit tests covering hosting WinForms controls then I could write some code for that.

This would be awesome to have!

weltkante · 2021-05-02T21:49:06Z

@RussKie sorry, didn't notice you were waiting with the PR for tests. As mentioned above I don't think this regression is testable in an automated fashion and my comment about testing ActiveX was about testing in general, not related to this PR - but if you want to wait with merging until we have a sample scenario to invoke for manual testing I can get to work and add a variant of the sandbox I linked above to the repo (probably a simplified version, there is no reason to use the full blown VS generated template for simple test scenarios).

RussKie · 2021-05-05T11:51:09Z

you want to wait with merging until we have a sample scenario to invoke for manual testing I can get to work and add a variant of the sandbox I linked above to the repo (probably a simplified version, there is no reason to use the full blown VS generated template for simple test scenarios).

Yes, please. A manually run test is just as good.

weltkante · 2021-05-05T22:42:00Z

Added the test scenario, but encountered some issues (you should be able to run it from VS in the x64 configuration)

x86 doesn't build because the .NET Core SDK can't figure out the right comhost dll
- I assume that is because your scripts only install the 64 bit SDK? or maybe its a bug in the SDK.
- I could disable x86 builds of this project in the solution if we don't need them?
the amd64 configuration uses the x64 build
- this is the same as the already existing native test project does, probably makes no sense and was missed when adding amd64 configurations
- I have no idea how to configure a VC++ project for amd64 as its not part of the default template
this project uses a VC++ project file (instead of CMake) which probably isn't installed on the CI build machines
- I don't know how to configure CMake projects, but I did setup the VC++ project to honor artifacts directory
- the native projects outputs itself into the bin folder of the managed control, so I don't have to copy the whole folder over into the native host output; it has its own obj build folder though
I have no idea how to enable CompModSwitches via configuration, so I had to add InternalsVisibleTo (which otherwise wouldn't be necessary)
- the CompModSwitches.ActiveX setting is required to see the debug output of the failing method call if the fix is not applied
as mentioned above MFC cannot cleanly shutdown its debug build when WinForms was loaded (an assert gets hit) so I added a workaround to the NativeHost code to kill itself on shutdown via TerminateProcess
- I assume this is because you'd want to force a GC to ensure all COM references have been released, but thats not easily doable

weltkante · 2021-05-05T22:47:25Z

Winforms.sln

+		{3129970B-A4EB-46AC-B163-18A5557B11BD}.Debug|Any CPU.ActiveCfg = Debug|x64
+		{3129970B-A4EB-46AC-B163-18A5557B11BD}.Debug|Any CPU.Build.0 = Debug|x64
+		{3129970B-A4EB-46AC-B163-18A5557B11BD}.Debug|arm64.ActiveCfg = Debug|x64
+		{3129970B-A4EB-46AC-B163-18A5557B11BD}.Debug|arm64.Build.0 = Debug|x64


this is weird but consistent with the other native project you're building

weltkante · 2021-05-05T22:47:49Z

src/System.Windows.Forms.Primitives/src/Properties/AssemblyInfo.cs

@@ -24,6 +24,7 @@
 [assembly: InternalsVisibleTo("MauiRichTextBoxTests, PublicKey=00000000000000000400000000000000")]
 [assembly: InternalsVisibleTo("MauiTestsHelper, PublicKey=00000000000000000400000000000000")]
 [assembly: InternalsVisibleTo("MauiTabControlTests, PublicKey=00000000000000000400000000000000")]
+[assembly: InternalsVisibleTo("NativeHost.ManagedControl, PublicKey=00000000000000000400000000000000")]


only necessary because I don't know how to set CompModSwitches via configuration settings

App.config did not work? winforms removed dependency but the functionality should be there.

I thought it was removed entirely, I'll try if it works

Nope, the instructions don't seem to work. I've tried several variations how to name the .config file, and I tried it in a standalone managed project instead of a native host, but neither seemed to work. If there are more up to date instructions I'm happy to try again. (And I made sure to also test the instructions on Desktop Framework, there they do indeed work.)

Strange. For this test project, we can keep as-is. I will play around with winforms projects to see if entire config is not working or some sections are not populated.

weltkante · 2021-05-05T22:48:02Z

src/System.Windows.Forms/tests/IntegrationTests/NativeHost.ManagedControl/ManagedControl.cs

+    {
+        public ManagedControl()
+        {
+            CompModSwitches.ActiveX.Level = System.Diagnostics.TraceLevel.Verbose;


it would be better to do this via some configuration, but how?

weltkante · 2021-05-05T22:48:37Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

@@ -13713,7 +13713,7 @@ unsafe HRESULT Ole32.IOleObject.GetMiscStatus(Ole32.DVASPECT dwAspect, Ole32.OLE
                return HRESULT.E_POINTER;
            }

-            if ((dwAspect & Ole32.DVASPECT.CONTENT) != 0)
+            if ((dwAspect & Ole32.DVASPECT.CONTENT) == 0)
            {
                Debug.WriteLineIf(CompModSwitches.ActiveX.TraceInfo, "AxSource:GetMiscStatus.  Status: ERROR, wrong aspect.");


Debug.WriteLineIf(CompModSwitches.ActiveX.TraceInfo, ...);

Note that observing this message without the above fix applied requires a debug build and running in a debugger. It can be tested by running the NativeHost from inside VS (flip the comparison back to test the original regression).

weltkante · 2021-05-05T22:49:28Z

src/System.Windows.Forms/tests/IntegrationTests/NativeHost/NativeHost.vcxproj

+    <OutDir>$(ArtifactsBinDir)NativeHost.ManagedControl\$(PlatformTarget)\$(Configuration)\$(TargetFramework)\</OutDir>
+    <IntDir>$(ArtifactsObjDir)$(ProjectName)\$(PlatformTarget)\$(Configuration)\$(TargetFramework)\</IntDir>


I'm recognizing artifcats folders. I'm outputting to NativeHost.ManagedControl to avoid copying the whole MangedControl folder into the NativeHost.

weltkante · 2021-05-05T22:50:59Z

src/System.Windows.Forms/tests/IntegrationTests/NativeHost/NativeHost.cpp

+		// a clean exit is currently not possible, we would have to force a garbage collection
+		// to ensure WinForms had a chance to release its COM objects before MFC exits
+		TerminateProcess(GetCurrentProcess(), S_OK);


In debug builds MFC cannot cleanly shutdown due to hitting an assert, WinForms still has references to MFC. I assume forcing a GC might help, but thats not easily doable. As a workaround I just terminate the process.

RussKie · 2021-05-11T13:02:15Z

Thank you. I was OOF for a week, and I'll try to review this is within fortnight.

dreddy-work · 2021-05-17T23:56:50Z

@weltkante , did you get a chance to look at build failures here?

weltkante · 2021-05-18T02:54:39Z

@dreddy-work I specifically called these out as open issues above, besides other things which don't immediately result in build failures

x86 doesn't build because the .NET Core SDK can't figure out the right comhost dll

I assume that is because your scripts only install the 64 bit SDK? or maybe its a bug in the SDK.

[...]

this project uses a VC++ project file (instead of CMake) which probably isn't installed on the CI build machines

[...]

I don't know how to resolve this myself; maybe this test project just needs to be excluded from CI builds somehow.

dreddy-work · 2021-05-19T19:34:32Z

@dreddy-work I specifically called these out as open issues above, besides other things which don't immediately result in build failures

Thanks. I missed part of it. We may need to get help from people who know infra better here. If these tests can be run against X64 successfully, i think we should unblock this PR and limit tests running to X64 arch only until then and open a new issue to track it.

AaronRobinsonMSFT · 2021-08-16T23:03:31Z

This is an SDK bug.

@jkoritzinsky Why does this work in the non-COM scenario? I've looked at the PR and it seems reasonable but the following statement is concerning without some more context.

but the build system doesn't even copy System.Drawing.Common.dll into the output path like it does for other projects that aren't setting true.

jkoritzinsky · 2021-08-16T23:06:15Z

This works in a non-COM scenario because the "CopyLocalLockFileAssemblies" property is set to true for executable projects, which I believe the other case is (I haven't looked closely at this again recently, but that's my recollection from when I first looked at this)

AaronRobinsonMSFT · 2021-08-16T23:11:30Z

@jkoritzinsky So this sounds like yet another one of those cases where a project is actually a self-hosting scenario (e.g., COM server) which is much closer to an EXE than a pure managed assembly project. Is that fair? If so, would it make sense to start building that sort of model in the SDK?

jkoritzinsky · 2021-08-16T23:14:27Z

The failing case here that hits the SDK issues isn't self-hosted though, it's a managed COM DLL that is hosted by a native C++ exe, which we already built a model for in the SDK. The "sdk bug" is a bug in this support we added back in .NET Core 3.x (the bug was probably introduced after 3.x).

RussKie · 2021-08-16T23:26:34Z

Thank you!

With the following change to the managed control project to run the native host again

diff --git a/src/System.Windows.Forms/tests/IntegrationTests/NativeHost.ManagedControl/NativeHost.ManagedControl.csproj b/src/System.Windows.Forms/tests/IntegrationTests/NativeHost.ManagedControl/NativeHost.ManagedControl.csproj
index d139f3387..8431433ff 100644
--- a/src/System.Windows.Forms/tests/IntegrationTests/NativeHost.ManagedControl/NativeHost.ManagedControl.csproj
+++ b/src/System.Windows.Forms/tests/IntegrationTests/NativeHost.ManagedControl/NativeHost.ManagedControl.csproj
@@ -14,4 +14,8 @@
     <ProjectReference Include="..\..\..\src\System.Windows.Forms.csproj" />
   </ItemGroup>
 
+  <PropertyGroup>
+    <EnableDynamicLoading>true</EnableDynamicLoading>
+  </PropertyGroup>
+
 </Project>

RussKie · 2021-08-17T00:44:06Z

@weltkante can I get you to rebase, and add the following to NativeHost.ManagedControl.csproj, so we can merge it before we branch off for .NET 7.0

  <!-- Workaround for https://github.com/dotnet/sdk/pull/19764 -->
  <PropertyGroup>
    <EnableDynamicLoading>true</EnableDynamicLoading>
  </PropertyGroup>

Also while you're at it, please fix this too:

…directory

RussKie · 2021-08-17T09:17:59Z

Thank you! Sorry it took so long.

weltkante requested a review from a team as a code owner March 14, 2021 18:26

ghost assigned weltkante Mar 14, 2021

weltkante mentioned this pull request Mar 16, 2021

Hosting UserControl via ActiveX does not work correctly #4370

Closed

RussKie added the waiting-review This item is waiting on review by one or more members of team label Mar 16, 2021

RussKie previously approved these changes Mar 29, 2021

View reviewed changes

RussKie added 📭 waiting-author-feedback The team requires more information from the author 💥 regression-preview Regression from a preview release area-COM and removed waiting-review This item is waiting on review by one or more members of team labels Mar 29, 2021

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 2, 2021

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 5, 2021

weltkante dismissed RussKie’s stale review via 02127de May 5, 2021 22:33

weltkante force-pushed the issue4370 branch from 21ce61c to 02127de Compare May 5, 2021 22:33

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 5, 2021

weltkante commented May 5, 2021

View reviewed changes

RussKie added the waiting-review This item is waiting on review by one or more members of team label May 11, 2021

RussKie force-pushed the issue4370 branch from 02127de to 08803f4 Compare June 16, 2021 08:01

jkoritzinsky mentioned this pull request Aug 16, 2021

Set the EnableDynamicLoading property before importing Microsoft.PackageDependencyResolution.targets dotnet/sdk#19764

Merged

RussKie removed the waiting-review This item is waiting on review by one or more members of team label Aug 16, 2021

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 17, 2021

weltkante force-pushed the issue4370 branch from 2e7ad1b to 8613b15 Compare August 17, 2021 08:31

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 17, 2021

weltkante force-pushed the issue4370 branch from 8613b15 to 2e7ad1b Compare August 17, 2021 08:32

weltkante and others added 8 commits August 17, 2021 10:42

fix inversion of conditional

e21a29a

add interactive test with native host for managed control

a49857a

enable CompModSwitches.ActiveX

fe02e8d

Remove xlf

fd86d55

Fix usings

c55ee54

disable building of NativeHost in CI

54fd70e

add missing targets

734d2dc

workaround for EnableComHosting not copying dependencies into output …

f18c0fe

…directory

weltkante force-pushed the issue4370 branch from 9abf403 to f18c0fe Compare August 17, 2021 08:44

This comment has been minimized.

Sign in to view

remove unnecessary using statement

0c160b7

This comment has been minimized.

Sign in to view

RussKie approved these changes Aug 17, 2021

View reviewed changes

RussKie merged commit c1889d8 into dotnet:main Aug 17, 2021

ghost added this to the 6.0 rc1 milestone Aug 17, 2021

weltkante deleted the issue4370 branch August 17, 2021 09:45

ghost locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inversion of conditional #4685

Fix inversion of conditional #4685

weltkante commented Mar 14, 2021 •

edited by ghost

Loading

weltkante commented Mar 14, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

hughbe commented Mar 15, 2021

weltkante commented Mar 15, 2021 •

edited

Loading

RussKie commented Mar 29, 2021

RussKie commented Mar 29, 2021

weltkante commented May 2, 2021

RussKie commented May 5, 2021

weltkante commented May 5, 2021 •

edited

Loading

weltkante May 5, 2021

weltkante May 5, 2021

dreddy-work May 12, 2021

weltkante May 13, 2021

weltkante May 13, 2021 •

edited

Loading

dreddy-work May 13, 2021

weltkante May 5, 2021

weltkante May 5, 2021 •

edited

Loading

weltkante May 5, 2021 •

edited

Loading

weltkante May 5, 2021

RussKie commented May 11, 2021

dreddy-work commented May 17, 2021

weltkante commented May 18, 2021 •

edited

Loading

dreddy-work commented May 19, 2021

AaronRobinsonMSFT commented Aug 16, 2021

jkoritzinsky commented Aug 16, 2021

AaronRobinsonMSFT commented Aug 16, 2021

jkoritzinsky commented Aug 16, 2021

RussKie commented Aug 16, 2021

RussKie commented Aug 17, 2021

This comment has been minimized.

This comment has been minimized.

RussKie commented Aug 17, 2021

		<OutDir>$(ArtifactsBinDir)NativeHost.ManagedControl\$(PlatformTarget)\$(Configuration)\$(TargetFramework)\</OutDir>
		<IntDir>$(ArtifactsObjDir)$(ProjectName)\$(PlatformTarget)\$(Configuration)\$(TargetFramework)\</IntDir>

Fix inversion of conditional #4685

Fix inversion of conditional #4685

Conversation

weltkante commented Mar 14, 2021 • edited by ghost Loading

Proposed changes

Customer Impact

Regression?

Risk

Before

After

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

weltkante commented Mar 14, 2021 • edited Loading

codecov bot commented Mar 14, 2021 • edited Loading

Codecov Report

hughbe commented Mar 15, 2021

weltkante commented Mar 15, 2021 • edited Loading

RussKie commented Mar 29, 2021

RussKie commented Mar 29, 2021

weltkante commented May 2, 2021

RussKie commented May 5, 2021

weltkante commented May 5, 2021 • edited Loading

weltkante May 5, 2021

Choose a reason for hiding this comment

weltkante May 5, 2021

Choose a reason for hiding this comment

dreddy-work May 12, 2021

Choose a reason for hiding this comment

weltkante May 13, 2021

Choose a reason for hiding this comment

weltkante May 13, 2021 • edited Loading

Choose a reason for hiding this comment

dreddy-work May 13, 2021

Choose a reason for hiding this comment

weltkante May 5, 2021

Choose a reason for hiding this comment

weltkante May 5, 2021 • edited Loading

Choose a reason for hiding this comment

weltkante May 5, 2021 • edited Loading

Choose a reason for hiding this comment

weltkante May 5, 2021

Choose a reason for hiding this comment

RussKie commented May 11, 2021

dreddy-work commented May 17, 2021

weltkante commented May 18, 2021 • edited Loading

dreddy-work commented May 19, 2021

AaronRobinsonMSFT commented Aug 16, 2021

jkoritzinsky commented Aug 16, 2021

AaronRobinsonMSFT commented Aug 16, 2021

jkoritzinsky commented Aug 16, 2021

RussKie commented Aug 16, 2021

RussKie commented Aug 17, 2021

This comment has been minimized.

This comment has been minimized.

RussKie commented Aug 17, 2021

weltkante commented Mar 14, 2021 •

edited by ghost

Loading

weltkante commented Mar 14, 2021 •

edited

Loading

codecov bot commented Mar 14, 2021 •

edited

Loading

weltkante commented Mar 15, 2021 •

edited

Loading

weltkante commented May 5, 2021 •

edited

Loading

weltkante May 13, 2021 •

edited

Loading

weltkante May 5, 2021 •

edited

Loading

weltkante May 5, 2021 •

edited

Loading

weltkante commented May 18, 2021 •

edited

Loading